Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update publish modal schema #728

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Yashsharma1911
Copy link
Member

@Yashsharma1911 Yashsharma1911 commented Sep 4, 2024

Notes for Reviewers

This PR fixes #

This PR adds missing parameter in publish modal RJSF schema which tell RJSF to decode input data in URI encoded format and show data in input field in decoded format

Signed commits

  • Yes, I signed my commits.

Signed-off-by: Yash sharma <[email protected]>
@Yashsharma1911
Copy link
Member Author

Yashsharma1911 commented Sep 4, 2024

From today’s call, there was some confusion about whether we should accept this change or not. I think yes, we should, because Uzair's proposal was to migrate these schemas to the schemas repo, which wasn't directly related to fixing the issue. My proposal was to address the issue first. Previously, we had these schemas in the schemas repo, but they were migrated to Sistent. Moving them back to the schemas repo now would require more effort than is currently necessary.

For now (right now) I'm focusing on fixing issue not creating extra work which I'm not sure should be a priority, I understand it is nice to have these schemas in schemas repo however I'm not sure if we should put this on priority right now, if we should do then let me know

@leecalcote
Copy link
Member

@MUzairS15 @aabidsofi19 thoughts?

},
pattern_info: {
type: 'string',
title: 'Description',
description: 'Purpose of the design along with its intended and unintended uses.',
format: 'textarea',
'x-rjsf-grid-area': 12
'x-rjsf-grid-area': 12,
"x-encode-in-uri": true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Yashsharma1911 these are native to rjsf right ? . if not where are we documenting these . we should be using a uiSchema and not putting the presentation specific properties in the data structure schema

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree @aabidsofi19 we can move it to uiSchema, we are tracking them here - https://github.com/layer5io/sistent/blob/master/src/schemas/readme.md#custom-properties

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also in RJSF section in Meshmap developer guide

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ops sorry, I mean "Kanvas"

@aabidsofi19
Copy link
Contributor

aabidsofi19 commented Sep 5, 2024

i think the publish schema should be derived from the schema for input of the publish endpoint . to remove the redundancy of redefining the same properties . for the presentation we should use a separate uiSchema ( that defines ui like the widget to use, grid size etc not the data structure )

// @leecalcote @Yashsharma1911 @MUzairS15

@leecalcote
Copy link
Member

A relevant discussion - meshery/meshkit#584 (comment)

@Yashsharma1911
Copy link
Member Author

i think the publish schema should be derived from the schema for input of the publish endpoint . to remove the redundancy of redefining the same properties . for the presentation we should use a separate uiSchema ( that defines ui like the widget to use, grid size etc not the data structure )

// @leecalcote @Yashsharma1911 @MUzairS15

This makes much more sense, we can update the capability of uiSchema to support this, and one part is to move them in single place, it does make sense for me if these schemas get bundled in schema repo npm package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants